Mysky autologin stored settings and logout #188

mrcnski · 2021-12-22T22:25:42Z

PULL REQUEST

Overview

Implements logout
Implements auto re-login
Implements getting user settings such as preferred portal and email
Implements most of the preferred portal flow
Moves portal login from MySky UI to Main MySky

Portal refresh flow

/**
*  Load MySky redirect flow:
*
*  1. SDK opens MySky on the same portal as the skapp.
*  2. If the preferred portal is found in localstorage, MySky connects to it
*     and we go to step 5.
*  3. Else, MySky connects to siasky.net.
*  4. MySky tries to get the saved portal preference.
*     1. If the portal is set, MySky switches to using the preferred portal.
*     2. If it is not set or we don't have the seed, MySky switches to using
*        the current portal as opposed to siasky.net.
*  5. After MySky finishes loading, SDK queries `mySky.getPortalPreference`.
*  6. If the preferred portal is set and different than the current portal,
*     SDK triggers refresh.
*  7. We go back to step 1 and repeat, but since we're on the right portal
*     now we won't refresh in step 6.
*
* Login redirect flow:
*
* 1. SDK logs in through the UI.
* 2. MySky UI switches to siasky.net and tries to get the saved portal
*    preference.
*    1. If the portal is set, MySky switches to using the preferred portal.
*    2. If it is not set or we don't have the seed, MySky switches to using
*       the current portal as opposed to siasky.net.
* 3. SDK queries `mySky.getPortalPreference`.
* 4. If the preferred portal is set and different than the current portal,
*    SDK triggers refresh.
* 5. We go to "Load MySky" step 1 and go through that flow, but we don't
*    refresh in step 6.
*/

TODO

Figure out how to get stored data to persist across portals during redirect. ( see below )
Still have to signal to MySky UI that we are done logging in.
Add dedicated flow(s) for changing the email after it's set. (in a future PR)

These TODOs are also documented in comments in the code.

Persisting data across portals

Persist settings from one portal to another when redirecting
- Check if email is valid first by trying to register/login
- Save everything in user settings
- Redirect
- Load email from user settings on MySky load and login again
- Save seed/email/preferred portal in local storage
- ******* How to send seed to MySky on new portal? *******

Testing

Of course this needs to be tested as well.

Test register + login flows
Make a skapp that sets preferred portal
Test preferred portal redirect

SDK PR

See SkynetLabs/skynet-js#361, which implements the page refresh in the SDK. These must be tested in tandem.

peterjan · 2022-01-13T13:35:40Z

scripts/ui.ts

@@ -117,6 +115,9 @@ async function requestLoginAccess(permissions: Permission[]): Promise<[boolean,
  // Save the seed and email in local storage.
  saveSeedAndEmail(seed, email);

+  // Wait for Main MySky to login successfully.
+  await waitForMySkyPortalLogin();


nit looks kind of weird to be naming an async function waitFor because you don't necessarily await it

peterjan · 2022-01-13T13:36:57Z

scripts/ui.ts

+ * Waits for portal login on Main MySky to complete.
+ */
+async function waitForMySkyPortalLogin(): Promise<void> {
+  return new Promise((resolve, reject) =>


this might block forever, that's ok right? would've expected to see a timeout mechanism of sorts here

peterjan · 2022-01-13T13:40:15Z

src/mysky.ts

+  JsonData,
+  deriveEncryptedFileKeyEntropy,
+  EncryptedJSONResponse,
+  MAX_REVISION,


what are all these GA failures about? (Module '"skynet-js"' has no exported member 'EncryptedJSONResponse'.)

You need to link to the dependency PR mentioned in the description.

peterjan · 2022-01-13T13:42:05Z

src/mysky.ts

+    const mySky = new MySky(initialClient, mySkyDomain, referrerDomain, permissionsProvider, preferredPortal);
+
+    // Login to portal.
+    {


this is weird? what are these curly braces all about?

I thought it made it more clear that the comment applies to the whole scope.

hm ok, kind of implies it should/could be a separate function but equally fine leaving this in I guess 🤷‍♂️

peterjan · 2022-01-13T13:46:26Z

src/mysky.ts

+
+    const methods = {
+      checkLogin: this.checkLogin.bind(this),
+      getEncryptedFileSeed: this.getEncryptedPathSeed.bind(this),


oof! should be covered with a unit test

Added a comment.

peterjan · 2022-01-13T13:50:06Z

src/mysky.ts

+    return { data: json };
+  }
+
+  // TODO


probably best to describe what's left to be done

peterjan · 2022-01-13T13:51:17Z

src/mysky.ts

+    const dataKey = deriveEncryptedFileTweak(pathSeed);
+
+    // Immediately fail if the mutex is not available.
+    return await this.client.db.revisionNumberCache.withCachedEntryLock(


it says revisionNumberCache is not defined? definitely issue with the typing here

This PR assumes the SkyDB rework has been merged in. The dependency PR is built on top of the latest changes in skynet-js.

peterjan · 2022-01-13T13:57:21Z

src/mysky.ts

+   * @param seed - The user seed.
+   * @param email - The user email.
+   */
+  protected setupAutoRelogin(seed: Uint8Array, email: string): void {


Not 100% sure I'm following here but this is a foot gun, or at least it looks like one.
Why not do this through some configuration variable or setting?

The implementation of executeRequest should simply have the error handling at all times and do:

if res.status == 401 && autoReloginEnabled { // login // retry } ...

I tried moving this logic into executeRequest as you suggested and it didn't seem like an improvement. If anything it just spread the logic out over two projects and coupled skynet-js even more closely with MySky. The way it is, the logic is confined to one place, and though it is slightly confusing I've seen much more confusing hook/callback type logic in Javascript...

I was debugging this code and I realized that your suggestion would actually simplify it (as well as fix my bug -- think I was missing a .bind(this) somewhere but that's not needed if the logic is already in client.executeRequest.) I pushed the change in the other PR. I called it loginFn though to hopefully not couple this too closely with "MySky auto-relogin".

peterjan · 2022-01-13T13:58:57Z

src/mysky.ts

+        this.client = getLoginClient(seed, null);
+
+        // Login to portal.
+        {


is this some kind of scope magic?

Yes, it's a block scope.

Was this confusing to you? I googled to see if it was standard and some people on StackOverflow were confused by it. Not sure how seriously to take that, though, because people on the internet have different experience levels. To my eyes this looks pretty clear. I wanted to avoid extracting this into a new function because having a lot of nested functions can also be hard to follow.

peterjan · 2022-01-13T14:10:28Z

src/mysky.ts

+            isEmailProvided = storedEmail !== null;
+          }
+
+          if (storedEmail) {


isEmailProvided is weird I think, I think I understand what you are doing but I would do this:

const storedEmail = email || checkStoredEmail(); if (storedEmail) { ... // If the email was found in local storage, save it in the user settings. if (!email) { await this.setUserSettings(seed, { portal: preferredPortal, email: storedEmail }); } }

this saves you a bunch of lines and a boolean, in any case I would rename isEmailProvided to something that explains why it will be used, or update the user settings inside of the if (!storedEmail) {, both would read easier

Renamed the variable and added an explanatory comment.

- [X] Fix domain extraction when on specific portal server - [X] Fix domain extraction for referrers - [X] Fix email not being saved in user settings (domain bug)

- [X] Fix portal account endpoints broken - [X] Fix bug with getting user settings (bad URL) - [X] Use `ensureUrl` when initializing client?

ro-tex · 2022-02-07T17:22:33Z

This is not exactly a code problem but we need to stick to some naming convention. Right now we have a bunch of kebab-case and a bunch of snake_case filenames, sometimes in the same directory.

…and-logout' into mysky-autologin-stored-settings-and-logout

mrcnski · 2022-02-07T21:52:27Z

@ro-tex Braised this for the file-naming issue: #241

ro-tex · 2022-02-07T17:05:49Z

package.json

    "crypto-browserify": "^3.12.0",
    "idb-keyval": "^6.1.0",
    "mustache": "^4.2.0",
    "post-me": "^0.4.5",
    "punycode": "^2.1.1",
    "randombytes": "^2.1.0",
-    "skynet-js": "^4.0.20-beta",
+    "skynet-js": "^4.0.22-beta",


We should update this to the latest version and proactively deal with any breaking changes.

This doesn't matter for now because I've linked to a specific branch in skynet-js locally. Changing this line wouldn't affect anything, I just have to keep the skynet-js branch up-to-date with recent changes. I will change this line once the skynet-js branch has been merged and a version change published.

ro-tex · 2022-02-08T14:57:29Z

scripts/ui.ts

+ *
+ * @returns - An empty promise.
+ */
+async function resolveOnMySkyPortalLogin(): Promise<void> {


This one took me a while to get it.
If I understand it correctly, what's happening is:

we register an event listener on storage which will resolve only if the storage action is called with key PORTAL_LOGIN_COMPLETE_SENTINEL_KEY and value 1

we also register a timeout that will reject the promise race after MYSKY_PORTAL_LOGIN_TIMEOUT if there hasn't been a valid login until then

So, the basic idea is that we wait for a login and if it doesn't happen withing a timeout, we reject.

If that's what's going on then it's fine. It's a little more complicated than I'd ideally like but I understand that the inter-process communication here is not exactly simple.

Questions:

Can we add a few more comments, outlining the ideas here? That actual registrations of listeners and so on are more than clear but the way the parts fit together in the big picture could be spelled out a bit more.

Having in mind that any JS on this page can write to the storage (please correct me if I'm wrong, my front end knowledge is not amazing), are we risking any security holes with this? Or is it that if there is malicious JS that can get to localStorage then we're dead anyway?

I will happily add more clarifying comments, thank you for the suggestion!

Regarding question 2: local storage is sandboxed per domain, so badapp.hns.siasky.net can't write to MySky's local storage. And MySky should only ever run its own code. If this invariant is ever broken, then yes: we're dead.

Thanks for explaining! One day I'll read all those fascinating things about browsers - storage types, workers, all of it. Until then I'll keep asking silly questions. ;)

src/skydb_internal.ts

src/provider.ts

src/mysky.ts

ro-tex

LGTM. All we need to do, from my perspective, is merge the skynet-js's new version and update package.json here. I'm holding off the approval until then but the code looks sound.

peterjan · 2022-02-14T12:12:06Z

html/seed-display.html

@@ -111,7 +111,7 @@ <h2 class="mt-6 text-center text-3xl font-extrabold text-gray-900">
      <!-- Signup form -->
      <div class="mt-8 sm:mx-auto sm:w-full sm:max-w-md">
        <div class="bg-white py-8 px-4 shadow sm:rounded-lg sm:px-10">
-          <form class="space-y-6" action="#" onsubmit="window.signUp()">
+          <form class="space-y-6" action="#" onsubmit="window.signUp(event)">


nit I think usually you want to call prevent default on the event, but you also want to return false here. I don't know why though unfortunately, I just know onsubmit="whatever(); return false" or onsubmit="return myValidator()" is a thing. Decided to leave the comment since it might be useful to you but feel free to just resolve if we don't care.

That sounds familiar to me, though I unfortunately don't understand what that does either. However,

I know that everything works the way it is now,

I would have to do all the deploys and testing again after making this change, and this was pretty annoying to test.

To me it makes sense to move this to a followup. Maybe at some point I can do a blitz through Issues with Karol or Farzad.

peterjan · 2022-02-14T12:15:51Z

scripts/ui.ts

+  const promise1 = new Promise<void>((resolve, reject) => {
+    const handleEvent = async ({ key, newValue }: StorageEvent) => {
+      if (key !== PORTAL_LOGIN_COMPLETE_SENTINEL_KEY) {
+        return;


so this makes it so promise1 never resolves, are we good with that?

Yeah, we only want the promise to resolve when the right storage key is encountered. Any other storage key shouldn't trigger a resolve.

Should we reject here, then?

No! 😅 We don't want login to fail if a different key is set for whatever reason. If there is another storage event listener for that key, it will also be triggered. Here, we are only interested in the specific key we are listening for.

peterjan · 2022-02-14T12:15:58Z

scripts/ui.ts

+      }
+      if (!newValue) {
+        // Key was removed.
+        return;


Added a comment.

src/skydb_internal.ts

peterjan

I'm not fully up to speed with the feature, but from a pure code standpoint this PR looks solid and I don't have any blocking comments, LGTM 👍

mrcnski added 6 commits December 15, 2021 15:53

Implement logout and setup auto re-login

7d2fe37

Fix up loose ends for logout and auto re-login

6a03865

Polyfill Promise.any for Opera

3ef9ad7

Implement getting preferred portal and redirect flow

5e60df4

Implement preferred portal flow, move portal login to Main MySky

6d10fe5

Fix a couple of lint warnings

a56b4fe

mrcnski added High Priority enhancement New feature or request labels Dec 22, 2021

mrcnski requested review from peterjan and ro-tex December 22, 2021 22:25

mrcnski added 4 commits December 22, 2021 16:31

Merge branch 'main' into mysky-autologin-stored-settings-and-logout

3db1e6a

Signal to MySky UI that we are done logging in

c1a7812

Refactor MySky get/setJSON based on GSJ rework

a8e518d

Address lint errors

181fd70

mrcnski marked this pull request as ready for review January 10, 2022 15:55

mrcnski added 2 commits January 12, 2022 13:41

Fix compilation issues

eae675b

Update tests to use siasky.net, remove references to SKYNET-TOKEN

10a9209

peterjan reviewed Jan 13, 2022

View reviewed changes

Address review comments

3d3ce52

mrcnski requested a review from peterjan January 13, 2022 20:46

mrcnski added 10 commits January 13, 2022 15:21

Merge branch 'main' into mysky-autologin-stored-settings-and-logout

cce3dbb

Fix build errors

6a19718

Add some fixes, debug lines, and refactors

bc5065a

Fix some bugs, add debug lines, send URL parameters to UI screens

d922f2d

Fix some bugs

1815b87

Refactor, get user settings in scripts/ui.ts

ccbf35f

Expose functions to allow skynet-js to auto-relogin

4034089

Fix domain extraction bugs (also fixing user settings bug)

a45f435

- [X] Fix domain extraction when on specific portal server - [X] Fix domain extraction for referrers - [X] Fix email not being saved in user settings (domain bug)

Fix bug in getJSONEncryptedInternal

50be38e

Fix referrer bug

4e10f5c

mrcnski added 4 commits February 2, 2022 16:22

Fix integration tests

fe13a7b

Don't make MySky initialization fail if the email is invalid

d5b79d1

Couple of critical fixes

1b44334

- [X] Fix portal account endpoints broken - [X] Fix bug with getting user settings (bad URL) - [X] Use `ensureUrl` when initializing client?

Merge branch 'main' into mysky-autologin-stored-settings-and-logout

21a0e4b

ro-tex and others added 3 commits February 7, 2022 18:22

Merge branch 'main' into mysky-autologin-stored-settings-and-logout

61602fb

Fix unexpected request with seed

49eebcc

Merge remote-tracking branch 'origin/mysky-autologin-stored-settings-…

5b2f738

…and-logout' into mysky-autologin-stored-settings-and-logout

mrcnski added 2 commits February 8, 2022 14:47

Merge branch 'main' into mysky-autologin-stored-settings-and-logout

7a76de1

Fix lint warnings

307b83a

ro-tex reviewed Feb 9, 2022

View reviewed changes

Address most review comments

58d6196

ro-tex self-requested a review February 11, 2022 15:48

ro-tex reviewed Feb 11, 2022

View reviewed changes

Comment resolveOnMySkyPortalLogin and unregister event listener

f9d0b59

peterjan reviewed Feb 14, 2022

View reviewed changes

src/skydb_internal.ts Show resolved Hide resolved

peterjan previously approved these changes Feb 14, 2022

View reviewed changes

mrcnski mentioned this pull request Feb 14, 2022

[nit] return false; when calling prevent default on event #252

Open

Add explanatory comments

f441ee5

mrcnski dismissed peterjan’s stale review via f441ee5 February 14, 2022 15:20

mrcnski requested review from peterjan and ro-tex February 14, 2022 16:07

Merge branch 'main' into mysky-autologin-stored-settings-and-logout

c951e5c

ro-tex approved these changes Feb 14, 2022

View reviewed changes

mrcnski merged commit ff8b549 into main Feb 15, 2022

Mysky autologin stored settings and logout #188

Mysky autologin stored settings and logout #188

Conversation

mrcnski commented Dec 22, 2021 • edited Loading

PULL REQUEST

Overview

Portal refresh flow

TODO

Persisting data across portals

Testing

SDK PR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro-tex commented Feb 7, 2022

mrcnski commented Feb 7, 2022

ro-tex Feb 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro-tex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterjan left a comment

Choose a reason for hiding this comment

mrcnski commented Dec 22, 2021 •

edited

Loading

ro-tex Feb 7, 2022 •

edited

Loading